Skip to content

K8s: Disable annotation when ingress.tls[0].secretName is null #2724

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Mar 21, 2025
Merged

K8s: Disable annotation when ingress.tls[0].secretName is null #2724

merged 1 commit into from
Mar 21, 2025

Conversation

@VietND96
Copy link
Member

@VietND96 VietND96 commented Mar 21, 2025 •
edited by qodo-merge-pro bot
Loading

User description

Thanks for contributing to the Docker-Selenium project!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines, applied for this repository.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Fixes #2720

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Enhancement, Tests

Description

  • Added conditional logic to disable proxy-ssl-secret annotation when ingress.tls[0].secretName is null.

  • Updated values.yaml to set useHttp2 default value to false.

  • Modified test configurations to include a secretName for TLS in the dummy solution.

Changes walkthrough 📝

Relevant files
Enhancement
_helpers.tpl
Refined logic for `proxy-ssl-secret` annotation inclusion

charts/selenium-grid/templates/_helpers.tpl

  • Adjusted conditional logic for proxy-ssl-secret annotation.
  • Ensured annotation is only added if ingress.tls[0].secretName is
    defined.
    		• +2/-2     
    Configuration changes
    values.yaml
    Updated default settings for `useHttp2` and comments         

    charts/selenium-grid/values.yaml

  • Changed default value of useHttp2 to false.
  • Updated comments for better clarity on sslSecret usage.
    		• +1/-1     
    Tests
    dummy_solution.yaml
    Updated test configurations for TLS `secretName`                 

    tests/charts/templates/render/dummy_solution.yaml

  • Added secretName to TLS configuration for testing.
  • Ensured test cases reflect new annotation logic.
    		• +1/-0     
    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @qodo-merge-pro
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    2720 - PR Code Verified

    Compliant requirements:

    • Add option to disable the nginx.ingress.kubernetes.io/proxy-ssl-secret annotation in selenium chart

    Requires further human verification:

    • Verify that the implementation works correctly when the TLS secret is created by cert manager

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    String Formatting Error

    The printf statement is missing the %s/%s format which could lead to incorrect path generation for the secret. The namespace and secret name need to be properly concatenated.

     
    nginx.ingress.kubernetes.io/proxy-ssl-secret: {{ tpl (printf "%s" $.Release.Namespace (index $.Values.ingress.tls 0).secretName) $ | quote }}
    Logic Change

    The PR changes the behavior from always adding the annotation to conditionally adding it. This might affect existing deployments that rely on the annotation being present.

     
        {{- else if (index $.Values.ingress.tls 0).secretName }}
nginx.ingress.kubernetes.io/proxy-ssl-secret: {{ tpl (printf "%s" $.Release.Namespace (index $.Values.ingress.tls 0).secretName) $ | quote }}

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Mar 21, 2025 •
    edited
    Loading

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix format string parameters

    The format string in the printf statement is missing format specifiers for the
    namespace and secretName values. This will result in only the namespace being
    included in the output, without the secretName.

    charts/selenium-grid/templates/_helpers.tpl [138-139]

     {{- else if (index $.Values.ingress.tls 0).secretName }}
-nginx.ingress.kubernetes.io/proxy-ssl-secret: {{ tpl (printf "%s" $.Release.Namespace (index $.Values.ingress.tls 0).secretName) $ | quote }}
+nginx.ingress.kubernetes.io/proxy-ssl-secret: {{ tpl (printf "%s/%s" $.Release.Namespace (index $.Values.ingress.tls 0).secretName) $ | quote }}
    • Apply this suggestion
    Suggestion importance[1-10]: 10

    __

    Why: This suggestion fixes a critical bug in the format string that would cause the secretName to be omitted from the proxy-ssl-secret annotation. The original code only has one "%s" placeholder despite passing two values (namespace and secretName), which would result in incorrect configuration and potential security issues.

    		High
    • Update

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Mar 21, 2025 •
    edited
    Loading

    CI Feedback 🧐

    (Feedback updated until commit 7ed1d75)

    A test triggered by this PR failed. Here is an AI-generated analysis of the failure:

    Action: Rerun workflow when failure

    Failed stage: Authenticate GitHub CLI for PR [❌]

    Failure summary:

    The action failed because the GitHub token provided to the workflow lacks the required read:org
    permission scope. The error occurred during the authentication step when trying to log in with the
    GitHub CLI using the token stored in the GH_CLI_TOKEN_PR environment variable. The specific error
    message was: "error validating token: missing required scope 'read:org'".

    Relevant error logs: 
    1:  ##[group]Operating System
2:  Ubuntu
...

22:  Issues: write
23:  Metadata: read
24:  Models: read
25:  Packages: write
26:  Pages: write
27:  PullRequests: write
28:  RepositoryProjects: write
29:  SecurityEvents: write
30:  Statuses: write
31:  ##[endgroup]
32:  Secret source: Actions
33:  Prepare workflow directory
34:  Prepare all required actions
35:  Getting action download info
36:  Download action repository 'actions/checkout@main' (SHA:85e6279cec87321a52edac9c87bce653a07cf6c2)
37:  Complete job name: Rerun workflow when failure
38:  ##[group]Run actions/checkout@main
...

42:  ssh-strict: true
43:  ssh-user: git
44:  persist-credentials: true
45:  clean: true
46:  sparse-checkout-cone-mode: true
47:  fetch-depth: 1
48:  fetch-tags: false
49:  show-progress: true
50:  lfs: false
51:  submodules: false
52:  set-safe-directory: true
53:  env:
54:  GH_CLI_TOKEN: ***
55:  GH_CLI_TOKEN_PR: ***
56:  RUN_ID: 13986152384
57:  RERUN_FAILED_ONLY: true
58:  RUN_ATTEMPT: 1
...

113:  Or undo this operation with:
114:  git switch -
115:  Turn off this advice by setting config variable advice.detachedHead to false
116:  HEAD is now at 7c25440 Merge 7ed1d75447dcc347fec0b9c8cd34e17ae62cf266 into 3a61a7c5ae5f1bb0d9d4373ab5bdd841567186cc
117:  ##[endgroup]
118:  [command]/usr/bin/git log -1 --format=%H
119:  7c25440159dbd3bab65942f863249c8cbc4f7be7
120:  ##[group]Run sudo apt update
121:  �[36;1msudo apt update�[0m
122:  �[36;1msudo apt install gh�[0m
123:  shell: /usr/bin/bash -e {0}
124:  env:
125:  GH_CLI_TOKEN: ***
126:  GH_CLI_TOKEN_PR: ***
127:  RUN_ID: 13986152384
128:  RERUN_FAILED_ONLY: true
129:  RUN_ATTEMPT: 1
...

170:  Reading state information...
171:  54 packages can be upgraded. Run 'apt list --upgradable' to see them.
172:  WARNING: apt does not have a stable CLI interface. Use with caution in scripts.
173:  Reading package lists...
174:  Building dependency tree...
175:  Reading state information...
176:  gh is already the newest version (2.68.1).
177:  0 upgraded, 0 newly installed, 0 to remove and 54 not upgraded.
178:  ##[group]Run echo "$GH_CLI_TOKEN_PR" | gh auth login --with-token
179:  �[36;1mecho "$GH_CLI_TOKEN_PR" | gh auth login --with-token�[0m
180:  shell: /usr/bin/bash -e {0}
181:  env:
182:  GH_CLI_TOKEN: ***
183:  GH_CLI_TOKEN_PR: ***
184:  RUN_ID: 13986152384
185:  RERUN_FAILED_ONLY: true
186:  RUN_ATTEMPT: 1
187:  ##[endgroup]
188:  error validating token: missing required scope 'read:org'
189:  ##[error]Process completed with exit code 1.
190:  Post job cleanup.

    @VietND96 VietND96 force-pushed the chart-tls-annotation branch from f615929 to 7ed1d75 Compare March 21, 2025 06:42
    @VietND96 VietND96 merged commit da16f65 into trunk Mar 21, 2025
    26 of 28 checks passed
    @VietND96 VietND96 deleted the chart-tls-annotation branch March 21, 2025 09:18
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    No reviews

    Assignees

    No one assigned

    Labels

    Review effort 2/5

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    [🚀 Feature]: Add option to disable nginx.ingress.kubernetes.io/proxy-ssl-secret annotation in selenium chart

    2 participants

    @VietND96